Skip to content

fix: stop CPU/memory blowup on non-git projects (#713, #937, #841)#1001

Open
bethzyy wants to merge 3 commits into
DeusData:mainfrom
bethzyy:fix/non-git-cpu-memory
Open

fix: stop CPU/memory blowup on non-git projects (#713, #937, #841)#1001
bethzyy wants to merge 3 commits into
DeusData:mainfrom
bethzyy:fix/non-git-cpu-memory

Conversation

@bethzyy

@bethzyy bethzyy commented Jul 10, 2026

Copy link
Copy Markdown

Fixes #713. Also addresses the non-git half of #937 / #841 (watcher re-index churn).

Non-git folders currently hit two independent resource-exhaustion bugs. They share one root cause — every guard in these paths assumes git answers for the project — so this PR fixes both; happy to split it into two PRs if you prefer (the mcp/discover and watcher changes are fully independent).

1. Memory: auto-index OOM guard never fires on non-git roots (#713)

maybe_auto_index counted files with git ls-files | wc -l:

  • In a non-git folder the count is 0, so auto_index_limit can never trigger and auto-index walks/indexes the entire tree — tens of GB of RAM on large code folders (auto_index uses tens of GB of memory in a non-git folder containing code #713).
  • On Windows the pipe was broken for everyone: cmd.exe passes single quotes literally and has no wc, so the guard silently returned nothing even for git repos.

Fix (src/mcp/mcp.c, src/discover/discover.{c,h}):

  • Count git ls-files lines in C — portable, no wc, double quotes.
  • When the git count is 0 (non-git root, or nothing tracked), fall back to a new cbm_discover_count_files(root, limit): a bounded native walk that honors the same skip dirs, suffix filters, and language gate as discovery and stops at limit + 1, so arbitrarily large trees cost O(limit) work. It deliberately does not load gitignore — over-counting only makes the guard more conservative, and the path must stay cheap.

2. CPU: watcher re-indexes forever when a tree stays dirty (#937/#841)

Two stacked defects in src/watcher/watcher.c:

  • is_git_repo() used git rev-parse --git-dir, which walks up the directory tree. A plain non-git folder nested under any unrelated ancestor repo is misclassified as git — and since every file in it is untracked in that ancestor, git status never comes back clean.
  • check_changes() treated "working tree is dirty" as "changed", so a persistently dirty tree triggered a full re-index on every poll cycle (5–60s), forever. For the nested non-git case this is a guaranteed infinite loop with zero user action; Watcher can repeatedly re-index dirty repos and cause large disk write amplification #937 measured >1 TB/day of disk writes from this class of churn.

Fix:

  • A folder with no .git entry of its own and zero tracked files is treated as non-git (watcher skips polling, per the module's design). An empty freshly-git init-ed root keeps its .git; a monorepo sub-package keeps its tracked files — both stay watched.
  • Boolean dirty check replaced with a status signature: FNV-1a over git status --porcelain --untracked-files=all -- . output, mixing in the mtime+size of each listed path. A poll re-indexes only when the signature moves:
    • a tree that merely stays dirty hashes identically → no churn;
    • a repeated edit to an already-dirty file changes mtime/size → still caught;
    • -- . scopes status to the watched subtree, so a monorepo package no longer churns on sibling changes;
    • the signature is committed only after a successful re-index, so failed runs retry;
    • submodule status is folded into the hash on POSIX (same coverage as the old submodule foreach check).

Tests

  • watcher/discover/gitignore/git_context (173) and mcp (145) suites pass under ASan+UBSan on Linux.
  • New: watcher_nested_non_git_skips (baseline logs strategy=none for a non-git folder under a git ancestor), discover_count_files_bounded (count, early exit, edge cases).
  • Updated: watcher_continued_dirty previously asserted the buggy every-poll re-index; it now asserts stay-dirty → no re-trigger, further edit → re-trigger.
  • E2E with the production binary: a 15-file non-git folder with auto_index_limit=10 logs autoindex.skip reason=too_many_files files=11 limit=10 (files=11 = early exit at limit+1); with limit=100 auto-index completes normally.

scripts/security-allowlist.txt updated for the renamed/added cbm_popen call sites in watcher.c (no new call classes).

🤖 Generated with Claude Code

…ta#937, DeusData#841)

Two independent resource-exhaustion bugs hit non-git folders:

1. Memory (DeusData#713): the auto-index file-count guard ran
   `git ls-files | wc -l`, which returns 0 in a non-git folder, so the
   auto_index_limit cap could never fire and auto-index would eat tens
   of GB on large code folders. The pipe was also broken on Windows
   (cmd.exe passes single quotes literally and has no `wc`).

   Fix: count `git ls-files` lines in C (portable), and fall back to a
   new bounded native walk - cbm_discover_count_files() - when the git
   count is 0 (non-git roots). The walk honors the same skip dirs,
   suffix filters, and language gate as discovery and stops at the
   limit, so huge trees cost O(limit) work.

2. CPU (DeusData#937/DeusData#841): the watcher treated "working tree is dirty" as
   "changed", so a tree that merely STAYED dirty re-indexed on every
   poll cycle (5-60s), forever. Worse, `git rev-parse` walks up the
   tree, so a plain non-git folder nested under an unrelated ancestor
   repo was misclassified as git - and, being entirely untracked, it
   looked permanently dirty: a guaranteed infinite re-index loop.

   Fix: (a) treat a folder with no .git of its own and zero tracked
   files as non-git (skip polling); an empty repo root keeps its .git,
   a monorepo sub-package keeps its tracked files, so both stay
   watched. (b) Replace the boolean dirty check with a status
   signature - an FNV-1a hash of `git status --porcelain
   --untracked-files=all -- .` output plus the mtime/size of every
   listed file. A poll triggers a re-index only when the signature
   MOVES, so persistent dirt no longer loops while repeated edits to
   the same file are still caught. Status is scoped to the watched
   subtree so a monorepo package does not churn on sibling changes.
   The signature is committed only after a successful re-index, so
   failed runs retry on the next poll.

Tests: watcher/discover/gitignore/git_context (173) and mcp (145)
suites pass under ASan+UBSan. New regression tests:
watcher_nested_non_git_skips, discover_count_files_bounded; the old
watcher_continued_dirty test asserted the buggy every-poll re-index
and now asserts the fixed behavior. E2E-verified with the production
binary: a 15-file non-git folder with auto_index_limit=10 now logs
autoindex.skip reason=too_many_files files=11 (early exit at
limit+1); with limit=100 auto-index proceeds normally.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: bethz <bethz@263.net>
@bethzyy
bethzyy requested a review from DeusData as a code owner July 10, 2026 11:16
bethzyy and others added 2 commits July 10, 2026 19:50
The new pending_status_sig member is longer than the previous longest
declaration, so clang-format's trailing-comment alignment moves every
comment in the struct block two columns right. Formatted with
clang-format 20 using the repo .clang-format; fixes the lint / lint CI
failure on PR DeusData#1001.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: bethz <bethz@263.net>
The status signature mixes each listed file's mtime/size via
stat(worktree_root + "/" + rel). worktree_root came from
`git rev-parse --show-toplevel`, but MSYS/Cygwin git prints
POSIX-mapped absolute paths (/tmp/...) that a native binary's stat()
cannot resolve, so on the CLANG64 CI runner every stat failed silently
and the mtime/size mix-in was lost. A second edit to an already-dirty
file then left the signature unchanged and no re-index fired -
watcher_continued_dirty failed at test_watcher.c:1008 (expected
index_call_count 2, got 1) on windows-latest/CLANG64.

Use `git rev-parse --show-cdup` instead: it prints the RELATIVE
walk-up from root_path to the toplevel (empty when root_path is the
toplevel), which we join onto root_path so the result always keeps the
caller's path form regardless of which git flavor answered. Verified
the output contract with git on Windows: toplevel prints an empty
line, a subdir prints "../../" (trailing slash trimmed before the
join).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: bethz <bethz@263.net>
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 10, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 10, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for tackling this resource-safety class. I have triaged it into 0.9.1-rc as high priority. Since it touches discovery, watcher behavior, MCP paths, and the security allowlist, review will focus on non-git guard behavior, watcher churn, the allowlist delta, and confirming there are no new file, process, or network side effects.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for the resource-safety work here — and a heads-up that the ground shifted under the watcher half while this sat.

The two fixes have diverged in reviewability, so I'd like to take you up on the split you offered:

1. Memory / non-git auto-index OOM (#713)discover.c/mcp.c, the git ls-files | wc -l → real file count fix. This half is clean, independent, and still exactly right. If you split it into its own PR I can review and merge it quickly — it's the higher-value, lower-risk half.

2. Watcher non-git churn (#937/#841 non-git half) — this now conflicts substantively with #1045 (merged since you opened this), which reworked the git dirty-detection path: git_is_dirty is gone, replaced by a content signature (git_dirty_signature = porcelain entries + per-file size/mtime), and baselines now commit only after a successful reindex. Your allowlist edit renaming git_is_dirtygit_status_signature and the watcher.c changes collide directly with that. The non-git gap you're addressing may well still be real (a non-git folder has no HEAD and no porcelain, so #1045's signature doesn't cover it), but it needs rebasing on top of #1045's rewrite rather than replacing it — please rebase the watcher half onto current main and reconcile with the new signature approach, so we keep both the git and non-git coverage.

The security-allowlist.txt change itself is benign (just cbm_popen call-site descriptions), no concern there.

Net ask: split #713 out for a fast merge; rebase + reconcile the watcher half against #1045 as a second PR. Happy to look at the watcher design with you if the non-git path is trickier than a straight rebase. Appreciate the depth on this class.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for this, and apologies for the slow first response. Queued for review.

One thing you can act on now: GitHub reports the branch as CONFLICTING against main, which has moved a long way since you opened it. CI is fully green (20 checks), so the conflict is the only mechanical blocker. A rebase would help — I would rather review the diff you intend than a stale one.

Non-git project handling touching three linked issues is a useful consolidation; I want to give it a proper read rather than a rushed one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auto_index uses tens of GB of memory in a non-git folder containing code

2 participants